-
Notifications
You must be signed in to change notification settings - Fork 176
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FEAT] NEAR-11: Landing Page Update #3116
Conversation
@hcho112 Thank you for your feedback, I've updated the PR based on your suggestions |
…logic with banner warning
c1d3309
to
5238392
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, but can we please delete the deprecated code instead of commenting out? It's hard to tell which routes are remaining 😅
import { SetupPassphraseNewAccountWrapper } from '../routes/SetupPassphraseNewAccountWrapper'; | ||
import { SetupRecoveryImplicitAccountWrapper } from '../routes/SetupRecoveryImplicitAccountWrapper'; | ||
import { SignWrapper } from '../routes/SignWrapper'; | ||
// import { CreateImplicitAccountWrapper } from '../routes/CreateImplicitAccountWrapper'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we delete these instead of commenting out please?
<GlobalAlert /> | ||
{ | ||
{/* { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please delete all commented-out routes so that it is clear exactly which routes remain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, just the one translation string that needs spaces. The rest of the feedback is just observations, not critical to shipping.
"landingSectionTitle": "What happened to NEAR Wallet?", | ||
"landingSectionSubTitle": "Transfer your accounts with ease.", | ||
"landingSectionSubDescription": "To make the transition easier, you can securely migrate your accounts to a new wallet using the Transfer Wizard. Review the transfer-compatible wallet options below or move your accounts manually with your recovery phrase.", | ||
"landingSectionDescription": "As we embrace a more decentralized future, the NEAR Wallet will be discontinued.This change invites you to discover a variety of new and secure wallet options within our ecosystem.Don’t worry, no changes will be made to your account or assets.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs spaces after periods
export function PageNotFound() { | ||
export function PageNotFound({ history }) { | ||
useEffect(() => { | ||
recordWalletMigrationEvent('REDIRECT'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth reporting the requested path here too? If we care enough about 404s from deprecated paths then that also seems relevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
const handleOnClickCreateNewAccount = () => { | ||
if (WEP_DISABLE_ACCOUNT_CREATION) { | ||
setShowModal('more-near-wallets'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The /create
route is gone, should this be the only statement in handleOnClickCreateNewAccount
i.e. show the modal with wallets?
Successfully merging this pull request may close these issues:
Replace Landing Page